[mypyc] fix: isinstance_native fast path conflict w/ interp. subclasses [1/1] #19957
Conversation
BobTheBuidler
changed the title
i think it would make sense for Edit: though looking at the internals of i see there's one more use of could you also add an IR build test with code that hits this bug? |
|
This is preventing me from updating one of my projects to 1.18, what would you say about cherry-picking this and the crash fix in #19960 to a 1.18.3? |
BobTheBuidler
changed the title
|
not sure if there's going to be a 1.18.3 since #19764 is closed but maybe you could suggest it there. |
| return f"{self.module_name}.{self.name}" | ||
|
|
||
| @property | ||
| def allow_interpreted_subclasses(self) -> bool: |
There was a problem hiding this comment.
I'm not entirely sure that this is correct in terms of the rest of the codebase right now, but semantically it feels like the correct thing to do
Any class with is_ext_class=False and is_final=False will inherently allow interpreted subclasses. But this new property bricked the codebase so clearly I'm misunderstanding something about how these 3 components interact
I can remove the property, fix just this one case, but then there will be other places in the codebase where allow_interpreted_subclasses could return False for a class that actually does allow interpreted subclasses. That's the state of things currently, but should it be?
| L0: | ||
| r0 = __main__.NonNative :: type | ||
| r1 = get_element_ptr x ob_type :: PyObject | ||
| r2 :: borrow load_mem r1 :: builtins.object* |
There was a problem hiding this comment.
this IR will change once we address the property question
2 participants
There is a bug in
isinstance_nativewhen one or more of the classes allows interpreted subclasses.When interpreted subclasses are allowed, we cannot make any assumptions about the number of subclasses at compile time.
I'm not sure if this fix is better suited in
all_concrete_classes